feat: data-attachment wrapper (Phase 2) - #13
Merged
Conversation
Add AttachmentRegistry/ArchieDataAttachment, wrapping Common Storage Lib's
DataManager to attach data to holders Archie doesn't own the class of -
Entity, BlockEntity, ItemStack, and (NeoForge only) ServerLevel - via a
stateless, reusable object rather than NBTHolder's per-instance-owned
storage. Declare via `AttachmentRegistry(modId) { val mana by intAttachment(...) }`,
use directly (`.get`/`.set`/`.has`/`.remove`/`.modify`) or as a delegate for
an extension property (`var Entity.mana by MyAttachments.mana`).
`sync` (reactive push to tracking players - Entity/BlockEntity on both
loaders, ServerLevel NeoForge-only) and `itemComponent` (vanilla
DataComponentType-backed, rides normal item replication instead) are kept
as two distinct, independently-toggleable mechanisms, matching what CSL's
real DataManagerBuilder actually requires under the hood (itemComponent
needs a client codec regardless of sync, confirmed against both platforms'
real DataManagerBuilderImpl source - passing null there breaks at
registration time).
Verified against CSL's actual source (both platforms' DataManagerBuilder/
DataManagerImpl, not just the bytecode signatures) rather than guessing:
- get()/has() exception behavior per holder kind, confirmed exact.
- A real, previously-undocumented platform quirk: get() on an unset
Entity/BlockEntity/ServerLevel value silently creates *and persists* the
default (Fabric's getAttachedOrCreate, NeoForge's getData both write
through on a miss) - has() can only tell "never touched" from "read once"
if called before the first get(). Documented in ArchieDataAttachment's
KDoc and docs/serialization.md; the new GameTest suite checks has()
before get() accordingly, after first hitting exactly this false
failure and confirming it against real Fabric behavior rather than
dismissing it as a test bug.
- ServerLevel sync is genuinely NeoForge-only (Fabric's updateTarget has
no Level case at all).
Also fixed a real Kotlin delegate-provider bug caught before it shipped:
returning ArchieDataAttachment<T> directly as a PropertyDelegateProvider's
own delegate type (since it itself implements ReadWriteProperty<Any?, T>)
made `val mana by intAttachment(...)` unwrap straight through to T instead
of binding mana's type to the attachment object - wrapping it in a plain
ReadOnlyProperty (matching NBTHolderImpl.itemField/fluidField/energyField's
existing pattern) fixes it. Isolated and confirmed via a standalone repro
before touching the real files.
New DataAttachmentTests/DataAttachmentTestFixtures in Archie-Test, using
vanilla Entity/BlockEntity/ItemStack fixtures throughout (attachments are
generic across holder kind, unlike Phase 1's capability lookups, so no
custom registered fixture type is needed). All 8 pass under a real
fabric-test:runGametest run.
Independent of Phase 1 (still open, unmerged) per the approved plan -
this branches off origin/1.21.x directly, so its test file uses plain
fail(...) checks rather than Phase 1's not-yet-merged public assertion
helpers.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: KP2048 <39203202+KP2048@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2 of the pre-release storage/GUI plan: data-attachment wrapper
Wraps Common Storage Lib's
DataManagerso Archie mods can attach persistent (and optionallysynced) data to holders they don't own the class of —
Entity,BlockEntity,ItemStack, and(NeoForge only)
ServerLevel— via a stateless, reusable object, complementingNBTHolder'sper-instance-owned field storage.
New API —
net.kernelpanicsoft.archie.serialization.AttachmentRegistry/ArchieDataAttachmentattachment(serializer, sync, copyOnDeath, itemComponent, default)(plus a reified variant andprimitive convenience wrappers -
intAttachment,stringAttachment, etc. - mirroringNBTHolder's field helpers).sync(reactive push to tracking players on every write) anditemComponent(vanillaDataComponentType-backed, rides normal item replication instead) are two genuinely differentmechanisms - confirmed against both platforms' real
DataManagerBuilderImplsource thatitemComponentneeds a client codec regardless ofsync, since CSL's own builder breaks atregistration time otherwise. Archie's wrapper always supplies one when needed, so this can't
bite a consumer.
MyAttachments.mana.get(holder)/.set/.has/.remove/.modify) or as anextension-property delegate, as above.
A real platform quirk, found and documented (not dismissed)
get()on an unset Entity/BlockEntity/ServerLevel value silently creates and persists thedefault - both Fabric's
getAttachedOrCreateand NeoForge'sgetDatawrite through on a miss.That means
has()can only tell "never touched" apart from "read once" if called before thefirst
get(). First surfaced as two real GameTest failures on Fabric; verified against Fabric'sactual attachment source rather than assumed, then documented in
ArchieDataAttachment's KDoc anddocs/serialization.md, and the test suite now checkshas()beforeget()accordingly.ItemStack/itemComponentholders don't have this quirk.Also confirmed (from CSL's real source, both platforms): ServerLevel sync is genuinely NeoForge-only
updateTargetdispatch has noLevelcase at all.A real bug caught before it shipped
Returning
ArchieDataAttachment<T>directly as aPropertyDelegateProvider's own delegate type(since it itself implements
ReadWriteProperty<Any?, T>so it can also back an extensionproperty) made
val mana by intAttachment(...)unwrap straight through toTinstead of bindingmana's type to the attachment object itself. Isolated via a standalone repro before touching thereal files; fixed by wrapping it in a plain
ReadOnlyProperty, matching the existing patternNBTHolderImpl.itemField/fluidField/energyFieldalready use for the same reason.Verification
New
DataAttachmentTests/DataAttachmentTestFixturesin Archie-Test, using vanillaEntity/BlockEntity/ItemStack fixtures throughout (attachments are generic across holder kind,
unlike Phase 1's capability lookups, so no custom registered fixture type is needed). All 8 pass
under a real
fabric-test:runGametestrun:neoforge-test:runGametestcurrently registers zero real tests for any mod in this environment(
GameTestHooks: Enabled Gametest Namespaces: [], before any of this PR's code runs) - apre-existing NeoForge GameTest task/environment gap, not something introduced here (Phase 1 never
exercised that task either). NeoForge-specific behavior here is instead verified directly against
CSL's real NeoForge source (
DataManagerImpl/DataManagerBuilderImpl), not just assumedsymmetric with Fabric.
Independent of Phase 1 (still open, unmerged) per the approved plan - this branches directly off
origin/1.21.x, so its test file uses plainfail(...)checks rather than Phase 1's not-yet-mergedpublic assertion helpers.
Part of the three-feature pre-release plan (capability lookup / data attachments / item-backed
container menus); Phase 3 to follow in a separate PR.
🤖 Generated with Claude Code